Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BIDS reading support and prepare input loading for pydra workflow #7

Merged
merged 17 commits into from
Oct 25, 2024

Conversation

maestroque
Copy link
Contributor

@maestroque maestroque commented Aug 28, 2024

Closes #9

Proposed Changes

  • Add a transform_to_physio pydra task to be used in all worfklows

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

physutils/physio.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
import logging

import pydra
Copy link
Member

@smoia smoia Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely adds complexity, but since physutils might be used by modules that are not based in pydra, could we try to create a safe import and make it optional, like the one we have for duecredit?

https://github.com/physiopy/phys2denoise/blob/master/phys2denoise/due.py

The idea would then to try from pydra.mark import task, but if it is not found, import task from a stub submodule of physutils. That task would be a decorator that returns the function.
(like here: https://github.com/BMRRgroup/wfTFI/blob/ec5a62cf2ffb347c752293e6234b7c89cebe711d/QSM.py#L7C1-L13C1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That task would be a decorator that returns the function

What do you mean here? I'm not sure I understand how you'd like this to be done (mostly given the duecredit example).

I tried doing smth like this, but it didn't really work out

def mark_task_wrapper(func):
    try:
        import pydra
        logger.debug("Pydra is installed and is currently enabled.")
        return pydra.mark.task(func)
    except ImportError:
        logger.warning("Pydra is not installed and is currently disabled. Please install it to use this module.")
        return func

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smoia feel free to check, I implemented it with just a wrapper and it seems to work

@me-pic
Copy link
Contributor

me-pic commented Aug 28, 2024

Other than the changes requested by Stef, LGTM !


@pydra.mark.task
def transform_to_physio(
input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None
Copy link
Member

@m-miedema m-miedema Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes a lot of sense to add this as a task, great addition to the phys2denoise workflow and to continue to build up physutils :)

I'd like to suggest changing the name of bids_channel to col_physio_type to increase consistency with the load_from_bids function - since there is not a lot of documentation within the code itself, I think it's important to try to maintain intuitive links!

Beyond that, I'm wondering why the task is named transform_to_physio when it can either transform to physio or read-in an existing physio object. Perhaps e.g. generate_physio would be more appropriate? At minimum I think it's helpful to describe the goal of this task and its potential use cases in the description for the PR. Also, please indicate the change type in the PR description :)

Other than these considerations and what Stef mentioned above regarding avoiding a dependency on pydra, this looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, it's ok as transform_to_physio, as even in case it reads a .phys file, it transforms a pickled Physio object file, into a Physio object instance to be used in further tasks.

Also maybe it'd be better to change the variable name in load_from_bids? It's not an interface variable (not used outside the function scope) so I don't think we should take this naming as a basis

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos on changing name to generate_physio or something similar.
Double kudos on adding a good docstring for explanations!

@maestroque
Copy link
Contributor Author

I also added an auto mode, as per physiopy/phys2denoise#57 (comment)
@smoia @me-pic @m-miedema

Comment on lines 21 to 34
def mark_task(pydra_imported=pydra_imported):
def decorator(func):
if pydra_imported:
# If the decorator exists, apply it
@wraps(func)
def wrapped_func(*args, **kwargs):
logger.debug(f"Creating pydra task for {func.__name__}")
return pydra.mark.task(func)(*args, **kwargs)

return wrapped_func
# Otherwise, return the original function
return func

return decorator
Copy link
Member

@smoia smoia Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may work, but I honestly was thinking about something like:

def task(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        return func(*args, **kwargs)
    return wrapper

In a utils module, and then here:.

try:
    from pydra.mark import task
except ImportError:
    from .utils import task

input_file: str, mode="physio", fs=None, bids_parameters=dict(), bids_channel=None
) -> Physio:
if not pydra_imported:
LGR.warning(
Copy link
Member

@smoia smoia Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LGR.warning(
LGR.debug(

setup.cfg Outdated
loguru
pydra
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to an extra

setup.cfg Outdated
loguru
pydra
pybids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, we need to move this to extras as well (it comes with SO MANY dependencies). Could you please open an issue to generically move as many dependencies as possible to extra dependencies? Not now, but it would be good to do so later.

@m-miedema
Copy link
Member

@maestroque do you need any help addressing @smoia's comments?

@smoia
Copy link
Member

smoia commented Oct 9, 2024

@maestroque any progress on this side?

@maestroque
Copy link
Contributor Author

@smoia @m-miedema I didnt have in mind that there was something pending for this sorry. As far as i can tell only the dependencies issue opening

Comment on lines 59 to 60
if not fs:
fs = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about these two lines, was there something you were trying to check?

@smoia
Copy link
Member

smoia commented Oct 9, 2024

@maestroque @m-miedema @me-pic I opened a PR to @maestroque 's branch: https://github.com/maestroque/physutils/pull/1/files https://github.com/maestroque/physutils/pull/2/files

It should take care of most issues. Emphasis on should.

@smoia
Copy link
Member

smoia commented Oct 9, 2024

And I also made a mess out of it - sorry. I shoudl have fixed things now, but the PR is #2 not #1

@smoia
Copy link
Member

smoia commented Oct 9, 2024

The only thing is that logging is still a bit weird cause it uses logging on one side and loguru on the other. Maybe we should just use logging for the moment?

A few mods stemming from PR convo
@smoia smoia changed the title Add transform_to_physio pydra task Add BIDS reading support and prepare input loading for pydra workflow Oct 12, 2024
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

input_file=bids_dir,
mode="bids",
bids_parameters=bids_parameters,
bids_channel="cardiac",
Copy link
Contributor

@me-pic me-pic Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing since bids_channel is not a valid argument in the generate_physio function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok just realized looking at the previous comments that bids_channel was changed to col_physio_type in generate_physio, but the test_tasks.py have not been update accordingly

Comment on lines +14 to +16
assert task.inputs.input_file == physio_file
assert task.inputs.mode == "physio"
assert task.inputs.fs is None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest is failing.. inputs is not an attribute included in the Physio class, nor input_file or mode...

Copy link
Contributor Author

@maestroque maestroque Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inputs is a pydra task attribute specifying the input struct of the task, it may be that the new changes making pydra optional broke the tests. Before the tests were successful

Copy link
Contributor

@me-pic me-pic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the tests in test_tasks.py are failing... Not sure I understand the inputs attribute. Also all the bids_channel need to be changed to col_physio_type

@maestroque
Copy link
Contributor Author

@me-pic @smoia please check the comments, unfortunately I cannot look into this right now. Before the changes it was ready however.

# from loguru import logger

try:
from pydra import task
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smoia Is it possible that here it should be from pydra.mark import task instead of from pydra import task ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that change test_generate_physio_bids_file and test_generate_physio_auto in test_tasks.py are now passing 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that could be it actually. Thank you for catching it!

@me-pic me-pic merged commit ab85017 into physiopy:master Oct 25, 2024
1 check passed
@smoia
Copy link
Member

smoia commented Oct 25, 2024

🚀 PR was released in 0.3.0 🚀

@smoia smoia added the released label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Move as many dependencies as possible to extra installs rather than normal ones
4 participants